Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove log stream and settings #204115

Merged
merged 25 commits into from
Jan 10, 2025
Merged

Remove log stream and settings #204115

merged 25 commits into from
Jan 10, 2025

Conversation

gbamparop
Copy link
Contributor

@gbamparop gbamparop commented Dec 12, 2024 •
edited
Loading

Removes the code used to render the log stream and settings pages.

The following areas have been checked:

  • Log stream embeddable (dashboard of the cisco_meraki integration)
  • Log stream shared component
  • Log categories
  • Log anomalies
Screen.Recording.2024-12-12.at.18.52.38.mov

Closes #204005

@gbamparop
Copy link
Contributor Author

/ci

@gbamparop gbamparop added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Dec 12, 2024
@gbamparop
Copy link
Contributor Author

/ci

@gbamparop
Copy link
Contributor Author

/ci

@gbamparop gbamparop mentioned this pull request Dec 19, 2024
Merged
This was referenced Dec 19, 2024
Closed
Closed
@gbamparop
Copy link
Contributor Author

/ci

@gbamparop
Copy link
Contributor Author

/ci

@gbamparop gbamparop marked this pull request as ready for review January 2, 2025 12:31
@gbamparop gbamparop requested a review from a team as a code owner January 2, 2025 12:31
@gbamparop gbamparop requested review from a team as code owners January 2, 2025 12:31
pheyos
pheyos approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-pack/test/upgrade/config.ts changes LGTM

@weltenwort weltenwort self-requested a review January 6, 2025 14:25
weltenwort
weltenwort requested changes Jan 7, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, that's a hefty number of removed lines 👏 I left just a few questions below.

If I'm not mistaken there are a few things in logs_shared we could also remove:

  • the routes in x-pack/platform/plugins/shared/logs_shared/server/routes/log_entries along with their API types in common
  • the component WithSummary exported by logs_shared/public

But feel free to delay that to a later PR if you prefer.

x-pack/platform/plugins/private/translations/translations/fr-FR.json Show resolved Hide resolved
x-pack/solutions/observability/plugins/infra/public/pages/logs/page_content.tsx Show resolved Hide resolved
@weltenwort
Copy link
Member

🤔 It's probably unrelated to these changes, but the shared Logs Stream component in APM trace details throw a Cannot read properties of undefined (reading 'euiColorEmptyShade') error for me.

@gbamparop
Copy link
Contributor Author

🤔 It's probably unrelated to these changes, but the shared Logs Stream component in APM trace details throw a Cannot read properties of undefined (reading 'euiColorEmptyShade') error for me.

This should be fixed by #205918

@gbamparop
Copy link
Contributor Author

gbamparop commented Jan 9, 2025 •
edited
Loading

Nice work, that's a hefty number of removed lines 👏 I left just a few questions below.

If I'm not mistaken there are a few things in logs_shared we could also remove:

  • the routes in x-pack/platform/plugins/shared/logs_shared/server/routes/log_entries along with their API types in common
  • the component WithSummary exported by logs_shared/public

But feel free to delay that to a later PR if you prefer.

Thank you for the review! More code was removed by 1072277

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1281 1233 -48
logsShared 377 358 -19
total -67

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
logsShared 286 230 -56

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.3MB 1.2MB -90.0KB
logsShared 281.2KB 286.2KB +5.0KB
total -85.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
logsShared 36 33 -3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 50.6KB 50.5KB -86.0B
logsShared 173.6KB 151.0KB -22.7KB
total -22.7KB
Unknown metric groups

API count

id before after diff
logsShared 315 256 -59

async chunk count

id before after diff
infra 30 29 -1
logsShared 16 15 -1
total -2

ESLint disabled in files

id before after diff
infra 9 8 -1

ESLint disabled line counts

id before after diff
infra 40 38 -2
logsShared 18 17 -1
total -3

Total ESLint disabled count

id before after diff
infra 49 46 -3
logsShared 21 20 -1
total -4

History

weltenwort
weltenwort approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for thoroughly following the unused imports as well!

I don't understand why the async logsShared bundles supposedly got larger due to this change. 🤷

MiriamAparicio
MiriamAparicio approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@MiriamAparicio MiriamAparicio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gbamparop gbamparop merged commit a3f07db into elastic:main Jan 10, 2025
9 checks passed
@gbamparop
Copy link
Contributor Author

I don't understand why the async logsShared bundles supposedly got larger due to this change. 🤷

cc @tonyghiani in case you have any thoughts as you were recently working on async bundles

@tonyghiani
Copy link
Contributor

cc @tonyghiani in case you have any thoughts as you were recently working on async bundles

My take is that something that was added to the initial bundle before is now only imported dynamically.
Maybe the runtime type logEntryFieldRT, which was used by some of the removed components and now is dynamically imported as part of the LogsAIAssistant chunk, but could be more stuff. It think it makes sense this happened.

CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
@gbamparop @kibanamachine @CAWilson94
Remove log stream and settings (elastic#204115)
f24d153 
Removes the code used to render the log stream and settings pages.

The following areas have been checked:
- Log stream embeddable (dashboard of the `cisco_meraki` integration)
- Log stream shared component
- Log categories
- Log anomalies


https://github.com/user-attachments/assets/2bc0763d-3def-4c4b-b50a-21c17976a596

Closes elastic#204005

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiriamAparicio MiriamAparicio MiriamAparicio approved these changes

@pheyos pheyos pheyos approved these changes

@weltenwort weltenwort weltenwort approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the code used to render the logs stream app and the logs settings page in Observability
7 participants
@gbamparop @weltenwort @elasticmachine @tonyghiani @pheyos @MiriamAparicio @kibanamachine